fix: do not pull in aws-lc-rs by default#1101
fix: do not pull in aws-lc-rs by default#1101thomaseizinger wants to merge 1 commit intogetsentry:masterfrom
aws-lc-rs by default#1101Conversation
|
This PR has been automatically closed. All non-maintainer contributions must reference an existing GitHub issue. Next steps:
Please review our contributing guidelines for more details. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| # transport settings | ||
| native-tls = ["dep:native-tls", "reqwest?/native-tls", "ureq?/native-tls"] | ||
| rustls = ["dep:rustls", "reqwest?/rustls", "ureq?/rustls"] | ||
| rustls = ["dep:rustls", "reqwest?/rustls-no-provider", "ureq?/rustls"] |
There was a problem hiding this comment.
Bug: The rustls feature now uses rustls-no-provider, requiring users to manually configure a crypto provider to avoid a runtime panic. This new requirement is not documented.
Severity: MEDIUM
Suggested Fix
Update the documentation for the rustls feature in sentry/lib.rs and sentry/README.md to explicitly state that users must install a crypto provider themselves. Provide a code example showing how to install a provider like ring or aws_lc_rs.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: sentry/Cargo.toml#L60
Potential issue: The `reqwest` dependency feature was changed from `rustls` to
`rustls-no-provider`. This intentionally removes the default crypto provider that was
previously included. As a result, users who enable the `sentry` crate's `rustls` feature
must now explicitly install a crypto provider (like `ring` or `aws_lc_rs`) in their
application's `main()` function. Without this manual step, the application will panic at
runtime when it attempts to make an HTTPS request. This new requirement is not
documented, potentially causing unexpected crashes for users upgrading the crate.
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1101 +/- ##
==========================================
+ Coverage 73.81% 74.29% +0.48%
==========================================
Files 64 67 +3
Lines 7538 7913 +375
==========================================
+ Hits 5564 5879 +315
- Misses 1974 2034 +60 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d0ea483. Configure here.
| # transport settings | ||
| native-tls = ["dep:native-tls", "reqwest?/native-tls", "ureq?/native-tls"] | ||
| rustls = ["dep:rustls", "reqwest?/rustls", "ureq?/rustls"] | ||
| rustls = ["dep:rustls", "reqwest?/rustls-no-provider", "ureq?/rustls"] |
There was a problem hiding this comment.
Rustls feature causes runtime panic without crypto provider
High Severity
Changing reqwest?/rustls to reqwest?/rustls-no-provider means the rustls feature no longer provides a working TLS backend for the reqwest transport. Users who enable rustls and disable native-tls (as the documentation at line 74 recommends) will hit a runtime panic at reqwest::Client::builder().build().expect(...) because no CryptoProvider is installed. The ring crate appearing in the workspace Cargo.lock is a red herring — it's pulled in by ureq?/rustls, but end-users who only enable reqwest won't have it. The existing docs and feature table still imply rustls is a drop-in TLS backend.
Reviewed by Cursor Bugbot for commit d0ea483. Configure here.
|
Superseded by #1103 |
2 participants
Description
Activating the
rustlsfeature ofreqwestpulls inaws-lc-rsby default as the crypto provider. To allow people to make their own decision about which crypto provider to use withrustls, we should depend on therustls-no-providerfeature flag instead.